-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove random functions in CMath #3906
remove random functions in CMath #3906
Conversation
src/shogun/base/init.cpp
Outdated
@@ -38,6 +37,7 @@ namespace shogun | |||
SGIO* sg_io=NULL; | |||
Version* sg_version=NULL; | |||
CMath* sg_math=NULL; | |||
__int32_t sg_random_seed = shogun::CRandom::generate_seed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, the logic in here is a bit of odd in here. I define the global random seed in init.cpp because here is where we define global variable. Then I do setter and getter function in SGObject in here due to we need to reference shogun/base/init.h every time if we want to call set_global_seed()
or getter otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why __
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the generate_seed function should not be part anymore of CRandom...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the generate_seed function should not be part anymore of CRandom...
So we need to move it to CSGObject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no i would put it into init.cpp as a static function
src/shogun/mathematics/Random.cpp
Outdated
#include <shogun/lib/Time.h> | ||
#include <shogun/lib/Lock.h> | ||
#include <shogun/mathematics/Math.h> | ||
#include <shogun/mathematics/Random.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vigsterkr, for using global random seed in default CRandom ctor. If I write it like https://gist.github.com/MikeLing/c5d618c176a14aec06f3411ce1b30c5e I will get a undefined symbols for architecture x86_64 c++ error. But everything works if I use CSGObject::get_global_seed() instead of sg_random_seed instead. I don't know why this happened :(
#include <shogun/mathematics/Math.h> | ||
#include <gtest/gtest.h> | ||
#include <shogun/mathematics/Random.cpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would you import Random.cpp here?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a mistake, let me remove it right away!
src/shogun/base/SGObject.h
Outdated
@@ -499,6 +501,16 @@ class CSGObject | |||
*/ | |||
void set_seed(int32_t seed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this you should drop
src/shogun/base/SGObject.h
Outdated
@@ -499,6 +501,16 @@ class CSGObject | |||
*/ | |||
void set_seed(int32_t seed); | |||
|
|||
static void set_global_seed(uint32_t seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need for this here... since you seed is a global thing.
just in init as other things have
set_global_seed
and get_global_seed
src/shogun/base/init.cpp
Outdated
@@ -38,6 +37,7 @@ namespace shogun | |||
SGIO* sg_io=NULL; | |||
Version* sg_version=NULL; | |||
CMath* sg_math=NULL; | |||
__int32_t sg_random_seed = shogun::CRandom::generate_seed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the generate_seed function should not be part anymore of CRandom...
1336aea
to
cfc41a6
Compare
@@ -325,6 +325,7 @@ SGVector<int32_t> CStatistics::sample_indices(int32_t sample_size, int32_t N) | |||
int32_t* idxs=SG_MALLOC(int32_t,N); | |||
int32_t i, rnd; | |||
int32_t* permuted_idxs=SG_MALLOC(int32_t,sample_size); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument as the default ctor uses global seed anyways right?
src/shogun/base/DynArray.h
Outdated
@@ -447,8 +448,12 @@ template <class T> class DynArray | |||
/** randomizes the array (not thread safe!) */ | |||
void shuffle() | |||
{ | |||
auto m_rng = | |||
std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -131,7 +131,7 @@ SGVector<int32_t> CKMeansMiniBatch::mbchoose_rand(int32_t b, int32_t num) | |||
{ | |||
SGVector<int32_t> chosen=SGVector<int32_t>(num); | |||
SGVector<int32_t> ret=SGVector<int32_t>(b); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom()); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -33,6 +33,7 @@ SGMatrix<float64_t> CDataGenerator::generate_checkboard_data(int32_t num_classes | |||
int32_t dim, int32_t num_points, float64_t overlap) | |||
{ | |||
int32_t points_per_class = num_points / num_classes; | |||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -86,12 +88,13 @@ SGMatrix<float64_t> CDataGenerator::generate_mean_data(index_t m, | |||
/* evtl. allocate space */ | |||
SGMatrix<float64_t> result=SGMatrix<float64_t>::get_allocated_matrix( | |||
dim, 2*m, target); | |||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -773,14 +775,15 @@ SGMatrix<float64_t> CStatistics::sample_from_gaussian(SGVector<float64_t> mean, | |||
|
|||
typedef SparseMatrix<float64_t> MatrixType; | |||
const MatrixType &c=EigenSparseUtil<float64_t>::toEigenSparse(cov); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
src/shogun/mathematics/ajd/QDiag.cpp
Outdated
@@ -16,6 +16,7 @@ SGMatrix<float64_t> CQDiag::diagonalize(SGNDArray<float64_t> C, SGMatrix<float64 | |||
int T = C.dims[2]; | |||
|
|||
SGMatrix<float64_t> V; | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
src/shogun/multiclass/LaRank.h
Outdated
return patterns[i]; | ||
} | ||
auto m_rng = | ||
std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -512,13 +512,13 @@ void Solver_MCSVM_CS::solve() | |||
} | |||
state->inited = true; | |||
} | |||
|
|||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
@@ -269,19 +269,23 @@ CHMSVMModel* CTwoStateModel::simulate_data(int32_t num_exm, int32_t exm_len, | |||
SGVector< int32_t > ll(num_exm*exm_len); | |||
ll.zero(); | |||
int32_t rnb, rl, rp; | |||
|
|||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(get_global_seed())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for the get_global_seed
argument
6061d2b
to
e4c4d96
Compare
So, in this pr, we had removed all static random generator and only share a static seed between different modules(because we want to make all the module have a fixed seed for the unit test). But, however, there are many tests(unit and meta test) failed for these changes. On locally, I have:
All of them are rely on global random which has been removed from this and last pr. We wondering if they failed because the side effect of global random removal and we could assume(believe) we haven't hurt those modules inside after apply this pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you agree that CRandom(sg_random_seed) and CRandom() will construct the same object with the same state? because it does...
so there's really no need for
new CRandom(sg_random_seed)) it's enough to call new CRandom() right?
@@ -325,6 +325,7 @@ SGVector<int32_t> CStatistics::sample_indices(int32_t sample_size, int32_t N) | |||
int32_t* idxs=SG_MALLOC(int32_t,N); | |||
int32_t i, rnd; | |||
int32_t* permuted_idxs=SG_MALLOC(int32_t,sample_size); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikeLing do you agree that CRandom(sg_random_seed)
and CRandom()
will construct the same object with the same state? because it does...
so there's really no need for
new CRandom(sg_random_seed))
it's enough to call new CRandom()
right?
@@ -711,12 +712,13 @@ SGMatrix<float64_t> CStatistics::sample_from_gaussian(SGVector<float64_t> mean, | |||
int32_t dim=mean.vlen; | |||
Map<VectorXd> mu(mean.vector, mean.vlen); | |||
Map<MatrixXd> c(cov.matrix, cov.num_rows, cov.num_cols); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
@@ -773,14 +775,15 @@ SGMatrix<float64_t> CStatistics::sample_from_gaussian(SGVector<float64_t> mean, | |||
|
|||
typedef SparseMatrix<float64_t> MatrixType; | |||
const MatrixType &c=EigenSparseUtil<float64_t>::toEigenSparse(cov); | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
src/shogun/mathematics/ajd/QDiag.cpp
Outdated
@@ -16,6 +16,7 @@ SGMatrix<float64_t> CQDiag::diagonalize(SGNDArray<float64_t> C, SGMatrix<float64 | |||
int T = C.dims[2]; | |||
|
|||
SGMatrix<float64_t> V; | |||
auto rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
src/shogun/multiclass/LaRank.h
Outdated
return patterns[i]; | ||
} | ||
auto m_rng = | ||
std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
@@ -33,6 +33,7 @@ SGMatrix<float64_t> CDataGenerator::generate_checkboard_data(int32_t num_classes | |||
int32_t dim, int32_t num_points, float64_t overlap) | |||
{ | |||
int32_t points_per_class = num_points / num_classes; | |||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
@@ -86,12 +88,13 @@ SGMatrix<float64_t> CDataGenerator::generate_mean_data(index_t m, | |||
/* evtl. allocate space */ | |||
SGMatrix<float64_t> result=SGMatrix<float64_t>::get_allocated_matrix( | |||
dim, 2*m, target); | |||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
@@ -107,7 +110,7 @@ SGMatrix<float64_t> CDataGenerator::generate_sym_mix_gauss(index_t m, | |||
/* evtl. allocate space */ | |||
SGMatrix<float64_t> result=SGMatrix<float64_t>::get_allocated_matrix( | |||
2, m, target); | |||
|
|||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
src/shogun/lib/SGVector.cpp
Outdated
@@ -614,8 +614,9 @@ void SGVector<float32_t>::vec1_plus_scalar_times_vec2(float32_t* vec1, | |||
template <class T> | |||
void SGVector<T>::random_vector(T* vec, int32_t len, T min_value, T max_value) | |||
{ | |||
auto m_rng = std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
src/shogun/mathematics/Math.h
Outdated
else | ||
{ | ||
auto m_rng = | ||
std::unique_ptr<CRandom>(new CRandom(sg_random_seed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new CRandom(sg_random_seed))
-> new CRandom()
@@ -223,4 +232,37 @@ namespace shogun | |||
} | |||
#endif | |||
} | |||
|
|||
uint32_t generate_seed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can actually replace this complicated function with a simple c++11 class std::random_device
... basically:
uint32_t generate_seed()
{
return std::random_device()();
}
d5cf7e6
to
48aa9bc
Compare
7a465c4
to
da08025
Compare
673d8e8
to
6920678
Compare
@@ -483,7 +483,7 @@ def translateExpr(self, expr): | |||
method = expr[key][0]["Identifier"] | |||
argsList = None | |||
try: | |||
argsList = expr[key][2] | |||
argsList = expr[key][1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @karlnapf , I already fix the missing parameter with global function call in here. Thank you for your help :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!!! :)
Hi @karlnapf , so we still have these meta tests failed:
Here is the generated data of gmm : https://pastebin.mozilla.org/9026925 Do we want to ensure all the difference is the side effect of global random and we don't need to worry about? |
@@ -287,7 +287,7 @@ TEST(KernelSelectionMaxCrossValidation, quadratic_time_single_kernel_dense) | |||
mmd->set_train_test_mode(false); | |||
|
|||
auto selected_kernel=static_cast<CGaussianKernel*>(mmd->get_kernel()); | |||
EXPECT_NEAR(selected_kernel->get_width(), 0.0625, 1E-10); | |||
EXPECT_NEAR(selected_kernel->get_width(), 0.03125, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -354,7 +354,7 @@ TEST(QuadraticTimeMMD, perform_test_permutation_biased_full) | |||
// assert against local machine computed result | |||
mmd->set_statistic_type(ST_BIASED_FULL); | |||
float64_t p_value=mmd->compute_p_value(mmd->compute_statistic()); | |||
EXPECT_NEAR(p_value, 0.0, 1E-10); | |||
EXPECT_NEAR(p_value, 0.8, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this change is OK.
0.0 essentially means that the test always works
0.8 means it almost never works.
Why would that come from a change of seed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same goes for all the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not because of change of seed then... hence my question here as i didn't want to dig into what is actually happening in this unit test.
i.e. this part needs to be reverted and figure out where else things get not properly done with the random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you re-run this a couple of times locally without a fixed seed, does the p-value fluctuate a lot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the job of an integration test, which we have. So I suggest to just drop these tests... Would you mind marking them? So @MikeLing can drop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karlnapf I think the following tests follow similar pattern:
TEST(QuadraticTimeMMD, perform_test_permutation_biased_full)
TEST(QuadraticTimeMMD, perform_test_permutation_unbiased_full)
TEST(QuadraticTimeMMD, perform_test_permutation_unbiased_incomplete)
TEST(QuadraticTimeMMD, perform_test_spectrum)
TEST(LinearTimeMMD, perform_test_gaussian_biased_full)
TEST(LinearTimeMMD, perform_test_gaussian_unbiased_full)
TEST(LinearTimeMMD, perform_test_gaussian_unbiased_incomplete)
TEST(KernelSelectionMaxMMD, linear_time_single_kernel_streaming)
TEST(KernelSelectionMaxMMD, quadratic_time_single_kernel_dense)
TEST(KernelSelectionMaxMMD, linear_time_weighted_kernel_streaming)
TEST(KernelSelectionMaxTestPower, linear_time_single_kernel_streaming)
TEST(KernelSelectionMaxTestPower, quadratic_time_single_kernel)
TEST(KernelSelectionMaxTestPower, linear_time_weighted_kernel_streaming)
TEST(KernelSelectionMaxCrossValidation, quadratic_time_single_kernel_dense)
TEST(KernelSelectionMaxCrossValidation, linear_time_single_kernel_dense)
TEST(KernelSelectionMedianHeuristic, quadratic_time_single_kernel_dense)
TEST(KernelSelectionMedianHeuristic, linear_time_single_kernel_dense)
However, I think instead of dropping all of these, some of these could be rewritten with hard coded data and asserted accordingly. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes absolutely!
What about creating a fixture class with a very simple and small fixed dataset, no randomness.
And then compute all the statistics and assert the results. Then put a link to a python notebook in there for reference. I did something like that here: https://github.com/karlnapf/shogun/blob/feature/kernel_exp_family/tests/unit/distribution/kernel_exp_family/impl/Nystrom_unittest.cc
and here
https://github.com/karlnapf/shogun/blob/feature/kernel_exp_family/tests/unit/distribution/kernel_exp_family/impl/DataFixture.h
As for the permutation test p-values. I would unit test the mechanics independently of ranodm data (if we have the statistics to be covered, we only need to test that the permutation test compares/computes the right things, i.e. test indices)
I think this would really benefit the maintainability of the framework!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karlnapf yeah unit-test fixture is a good idea. I agree, that for permutation tests we just need to check the indices. If the internal classes don't provide required APIs to access those things, we can use mock objects to allow access to the unit-tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First patch. Add meta example integraiton tests (minimal!) for all the unit test cases here. Then remove the unit tests
@@ -393,7 +393,7 @@ TEST(QuadraticTimeMMD, perform_test_permutation_unbiased_full) | |||
// assert against local machine computed result | |||
mmd->set_statistic_type(ST_UNBIASED_FULL); | |||
float64_t p_value=mmd->compute_p_value(mmd->compute_statistic()); | |||
EXPECT_NEAR(p_value, 0.0, 1E-10); | |||
EXPECT_NEAR(p_value, 0.8, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -432,7 +432,7 @@ TEST(QuadraticTimeMMD, perform_test_permutation_unbiased_incomplete) | |||
// assert against local machine computed result | |||
mmd->set_statistic_type(ST_UNBIASED_INCOMPLETE); | |||
float64_t p_value=mmd->compute_p_value(mmd->compute_statistic()); | |||
EXPECT_NEAR(p_value, 0.0, 1E-10); | |||
EXPECT_NEAR(p_value, 0.6, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -475,15 +475,15 @@ TEST(QuadraticTimeMMD, perform_test_spectrum) | |||
// assert against local machine computed result | |||
mmd->set_statistic_type(ST_BIASED_FULL); | |||
float64_t p_value_spectrum=mmd->compute_p_value(mmd->compute_statistic()); | |||
EXPECT_NEAR(p_value_spectrum, 0.0, 1E-10); | |||
EXPECT_NEAR(p_value_spectrum, 0.8, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
// unbiased case | ||
|
||
// compute p-value using spectrum approximation for null distribution and | ||
// assert against local machine computed result | ||
mmd->set_statistic_type(ST_UNBIASED_FULL); | ||
p_value_spectrum=mmd->compute_p_value(mmd->compute_statistic()); | ||
EXPECT_NEAR(p_value_spectrum, 0.0, 1E-10); | ||
EXPECT_NEAR(p_value_spectrum, 0.8, 1E-10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikeLing as for the
I think all of those contain random elements, so the integration tests indeed need to be updated if you change the seed. |
Hi @vigsterkr and @karlnapf , I changed these tests' expected output depend on linux(both gcc and clagn), but the window's random implementation looks like different :( |
The most annoying solution I can think of is using |
ok so i mean we are not thaaaaaaaaaaat bad (although of course note that good either). so basically i've just tested and as it is specified in the standards the random engines (mt19937, mt19937_64, minstd_rand etc) are generating the same values with the same order on the same seed :) of course it's annoying now that we'll have to provide our distribution mappers (like uniform, normal etc) but still we can do that - would be nice though to use the std api - and then have that as our random. still we can this way through out the external random generators that i've pulled in couple of years ago and still we are good with removing the global random :) |
I just test it on Linux(based on gcc, run on CLion) and window(on visual studio), and the results displayed totally different https://gist.github.com/MikeLing/12cf9254248e9ce49d343f297e920e1e#gistcomment-2154781 |
@MikeLing :) cool console!
|
based on the standard this should be the same regardless of the compiler/os/etc. |
so bascially the idea is that we can throw out it's a bit unfortunate as it would have been great to be able to use standards all the way, but.... :) on the other hand i'm just saying that we only require these distributions having the very same order of the very same values because of our tests, right? conceptually the model itself suppose to work correctly if it's getting the samples of the distribution it expects (regardless of the actual values of those samples... that's why it uses a prng and not a fixed set of integers in the first place)... it's only ourselves, who wanna be able to test that the model is producing the very same thing with the same samples (when setting the seed)... only thing that our fixtures in the tests are for a given sample-set of a distribution on a given OS/distribution... |
i've just realised that say we use c++11's random features all the way in the implementation and we somehow solve the problem of testing. i'm suspecting that of course one could still argue that if we do this the problem could be that one would end up with different models using the same seed depending on the compiler OS, or? question of course is that would those model be sooooo different? on the other hand i think it's a fair presumption from the user that regardless of your compiler/system etc for the same (data, prng seed) pair you wanna end up having the same model, or? |
Hi @vigsterkr , I think the root of this problem is about the use 'random' generated data in our unit test. Could we manually create some fixed mock data and use them instead of data generator like CMeanShiftDataGenerator or others? |
I mean it's a feature branch anyway, so we can use result of one platform like travis-ci to verify our refactor and add fixture or some other things to make them works on different system before merge it? |
@MikeLing mmmm not really... actually using CMeanShift and others would be in a way better solution than using mock data... as mock data either you have for each and every different platform/compiler... in case of random generator you can rely on the fact that they are consistent within the platform :) btw: have you checked whether that function i pasted above generates the same set and order of elements on windows? on osx and linux it's:
|
@vigsterkr Hi, sorry for the late reply. Here is the output I got on window(Visual studio 2015 and window10),
update: Oh! I got same output on Linux(ubuntu17.10, CLion based on gcc) :) |
I agree viktor, not using half of c++11 because of cross platform testing is not a good idea. I actually have an idea. What if we create a fake drop-in replacement for random (and all the distributions) that behaves deterministically. This would be purely for unit testing (not statistical testing) so it doesn't matter if the numbers are actually not "correct" Then we can have statistics tests (algorithm output "makes sense") which do not rely on fixed seeds |
@MikeLing ok wait this is weird. as they not only have different value but the decimal point as well... we should check on this because based on c++11 standards the random generators like std::mt19937_64 should actually be the same. |
okey! it's me who has been the idiot here! the output that i've pasted was for
|
hi @karlnapf , could you tell me more about the replacement? Let's say we want to replace all the number greater than 10 to 5, and replace all the number smaller than 1 to 0? |
@MikeLing are we ready with porting everywhere using |
Hi @vigsterkr , sorry for the late reply. mmm, if you mean if we can apply C++11 feature in everywhere except get the 'expected' result in the unit test, I would say not really, the NeuralNetwork test failed in a different way. And it seems like a fundamental error not some side effect. Basically speaking the NeuraNetwork don't work at all https://pastebin.mozilla.org/9027272. And I also do some research about it, I found the error comes from this line. Normally, it should change the m_parmater like:
But right now nothing changed after call train_lbfgs,
And furthermore, I found is because here, the rate somehow smaller than param.delta and force the loop break early then usual. Let me prove it than: Here is the result after refactor:
I don't know why we got this due to I only change the lay initiation with different random generator. We could discuss it later after I push it to the remote. Anyway, the lbfgs has too many pointers and c style things like goto. Sometimes I even don't know where the next point I should jump to, maybe we want a refactor for that? Until now, that's only broken module after move to C++11 |
@MikeLing have you tried changing the seed of the prng to something else than now? ok so now we have then everything using c++11 random, right? based on travis apart from the neural net errors the following tests fail:
|
@vigsterkr , actually if we change the |
@MikeLing is there a chance that you could test them on a linux machine using gcc? like docker on osx? |
@vigsterkr ok, so I get different value on osx and Linux(gcc-6.3.0) for integration tests :( Here is the result on Linux:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some changes required
@@ -42,7 +43,7 @@ CGMM::CGMM(int32_t n, ECovType cov_type) : CDistribution(), m_components(), m_co | |||
SG_REF(m_components[i]); | |||
m_components[i]->set_cov_type(cov_type); | |||
} | |||
|
|||
m_rng = get_prng<std::mt19937_64>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should fully remove this right?
@@ -91,7 +92,7 @@ CGMM::CGMM(vector<CGaussian*> components, SGVector<float64_t> coefficients, bool | |||
m_coefficients[i]=coefficients[i]; | |||
} | |||
} | |||
|
|||
m_rng = get_prng<std::mt19937_64>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does GMM really need an object level random?
float64_t rand_num=CMath::random(float64_t(0), float64_t(1)); | ||
|
||
std::normal_distribution<float64_t> normal_dist(0, 1); | ||
float64_t rand_num = normal_dist(m_rng); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really wanna use here an object level PRNG?
@@ -247,6 +248,8 @@ class CGMM : public CDistribution | |||
std::vector<CGaussian*> m_components; | |||
/** Mixture coefficients */ | |||
SGVector<float64_t> m_coefficients; | |||
|
|||
std::mt19937_64 m_rng; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is required the way we do GMM?
@@ -241,6 +241,8 @@ class CGaussian : public CDistribution | |||
SGVector<float64_t> m_mean; | |||
/** covariance type */ | |||
ECovType m_cov_type; | |||
|
|||
std::mt19937_64 m_rng; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because we need an object level rng?
@@ -167,6 +168,7 @@ SGVector<float64_t> CGaussianDistribution::log_pdf_multiple(SGMatrix<float64_t> | |||
|
|||
void CGaussianDistribution::init() | |||
{ | |||
m_rng = get_prng(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be explicit which of the PRNG engine we wanna use...
m_rng = get_prng<mt19937_64>();
@@ -73,6 +73,7 @@ void CGaussianBlobsDataGenerator::init() | |||
m_stretch=1; | |||
m_angle=0; | |||
m_cholesky=SGMatrix<float64_t>(2, 2); | |||
m_rng = get_prng(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explicitly specify the prng std::mt19937_64
@@ -50,10 +50,10 @@ void CMeanShiftDataGenerator::init() | |||
SG_ADD(&m_dimension_shift, "m_dimension_shift", "Dimension of mean shift", | |||
MS_NOT_AVAILABLE); | |||
|
|||
m_rng = get_prng(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explicitly define the required prng std::mt19937_64
src/shogun/mathematics/Math.h
Outdated
*/ | ||
template <class T> | ||
static void permute(SGVector<T> v, CRandom* rand=NULL) | ||
template < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should not be in math.... maybe we keep CRandom for that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vigsterkr , I somehow miss this comment. Actually this function is a part of math before refactor, I just move its implementation to .h due to we need use template parameter in it. So do we need to move it to CRandom and keep it(the random class) ?
src/shogun/neuralnets/RBM.cpp
Outdated
@@ -619,6 +623,7 @@ void CRBM::init() | |||
m_visible_state_offsets = new CDynamicArray<int32_t>(); | |||
m_num_params = 0; | |||
m_batch_size = 0; | |||
m_rng = get_prng(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be explicit of the PRNG type
@MikeLing ok so those errors are because of the discovered difference between the different implementations of the sampling functions, like |
Hi @vigsterkr , those sample functions been defined as virtual or pure virtual function(like CTraceSampler::sample(index_t) in the first place. So, I can't add template parameters into it directly or we want to use something like type_ensure(http://www.artima.com/cppsource/type_erasure.html) to make compiler know the template type in the first place. So I still use m_rng but only define and use it in sample functions. Does it make sense to you? :) |
@MikeLing i'm not so sure if i understand ;) |
Hi @vigsterkr , sorry for the late reply. So the problem for adding template parameter into sample function can be separated into two parts:
CNormalSample and CProbingSampler are inherited from CTraceSampler which is a pure virtual class. So, if we add template parameter into it, the compiler will complain like https://pastebin.mozilla.org/9027994. And we can't remove the virtual declaration since CTraceSampler is a pure vitual class after all.https://pastebin.mozilla.org/9027995
The compiler like the template parameter, but our meta test look it in the other way. When we use it like
|
ok so since this goes into a feature branch i'm merging this and rebasing it... then i'll try to come up with a solution for the fixtures |
No description provided.